add default.nix which can produce ps and pdf files #52
Conversation
| ''; | ||
| }; | ||
| in | ||
| stdenv.mkDerivation { |
There was a problem hiding this comment.
Why not just put renderBooklet as the derivation result? Wouldn't that make it easier to check the result into git? We can do nix-build -o SSS32.ps and then commit SSS32.ps. (I haven't actually tried this).
There was a problem hiding this comment.
Because I eventually want to produce more than one output -- the full booklet, but also postscript files with individual worksheets, 256-bit vesrions of the worksheets, etc. And maybe a illustrated version alongside the regular version.
There was a problem hiding this comment.
On further reflection, and having learned Nix better, let me explore making this derivation have multiple outputs, where the default is the fully-assembled SSS32.ps. I'm unsure if this is the right way to go still, because there are e.g. flags we want to be able to set about PDF vs PS, color vs non-color etc, and it might still be the cleanest thing to output a directory with a bunch of different files.
There was a problem hiding this comment.
My experiments with git and symlinks was a failure. While we could support multiple outputs, I do not think that it worth worrying about. At least not at this time.
apoelstra
force-pushed
the
2023-03--nixify
branch
2 times, most recently
from
d6cf782 to
865b397
Compare
|
Force pushed to a .nix which more closely matches the file I'm actually working with to add the full booklet contents, color, etc. You can see how the dependency management is going to work, once we break up the setup code into more than a single file, and we output the full booklet, but it's hopefully clearer how we'd output other documents too. |
|
9946318 seems to add an extra blank line on line 2060 for no good reason. |
default.nix
Outdated
| exit 1 | ||
| fi | ||
| } | ||
| '' + lib.optionalString doPageChecks toString ( |
There was a problem hiding this comment.
Should this all be in a standard checkPhase gated by a standard doCheck flag?
There was a problem hiding this comment.
Ah, yes, it should be.
| shortId = lib.optionalString (! isNull ref) ("-" + builtins.substring 0 8 src.rev); | ||
|
|
||
|
|
||
| setup = rec { |
There was a problem hiding this comment.
probably best to remove rec.
There was a problem hiding this comment.
I can remove it for now, but it will be used -- this setup set will contain all the various components of the postscript "setup" section. Each will have a dependencies array which refers to other setup components.
default.nix
Outdated
| ${lib.optionalString (pageData ? drawPageContent) "end\n"}pgsave restore | ||
| showpage | ||
| ''; | ||
| nextCtIdx = content.nextCtIdx + (if pageData ? drawPageContent then 1 else 0); |
There was a problem hiding this comment.
Should CtIdx just be PgIdx - 1, or is that just a coincidence?
There was a problem hiding this comment.
It's just a coincidence, but let me check my current development branch to see if we want this calculation at all. It originates with the PostScript originally having a giant array of page content in the setup section, but now that we're assembling pages in Nix, I'm going to move the content into their respective pages rather than indexing into an array.
Regarding the page numbering logic, it's going to wind up being a little ugly, since we need to support
- Pages that get numbers
- Pages that are numbered, but the number isn't displayed because it'd interfere with the content (at least one volvelle page are like this)
- Pages that aren't numbered at all
I'd also like to support some preamble pages having numbers like i, ii, iii, etc.
Anyway for now maybe I just want to rip out the page numbering logic entirely since we need to iterate on it.
|
@roconnor-blockstream I'll check on the "no good reason" blank line while in flight today, but I suspect that it's there because Nix multiline strings always end in a newline (or is it that they start with a newline? I forget), so if we don't have these newlines in the target output, it forces you to cram lots of stuff onto single lines. Edit no, this one was indeed for "no good reason" :) I'll remove it. |
apoelstra
force-pushed
the
2023-03--nixify
branch
from
8419d4c to
f3d8d3b
Compare
|
@roconnor-blockstream pushed new version which
|
| %* | ||
| %**************************************************************** | ||
| '' + '' | ||
| %%Page: ${toString content.nextPgIdx} ${toString content.nextPgIdx} |
There was a problem hiding this comment.
Perhaps this is for a follow up PR, but if you want to get fancy with page numbering then you need to update this line. This isn't just a pair of page numbers repeated for no reason. You'll have to look up the spec, but it is something like one is the literal sequence number and the other is the displayed page number. This makes a difference for assembling a sidebar of contents on your PS viewer.
There was a problem hiding this comment.
@roconnor-blockstream yep, I'll address that in a future PR. Unfortunately ps2pdf loses this information (or maybe PDF doesn't support it) so it's of somewhat limited value.
default.nix
Outdated
| mkdir "$out" | ||
| cd "$out" | ||
| cp ${renderBooklet fullBooklet} SSS32.ps |
There was a problem hiding this comment.
Should this be a symlink instead?
There was a problem hiding this comment.
Hmm, yeah, I guess it could be. It didn't occur to me.
| cp ${renderBooklet fullBooklet} SSS32.ps | ||
| ${lib.optionalString doOutputDiff "diff -C 5 ${src}/SSS32.ps SSS32.ps"} | ||
| sed -i 's/(revision \(.*\))/(revision \1${shortId})/' ./SSS32.ps |
There was a problem hiding this comment.
This probably needs to be more robust, but it is fine for now.
default.nix
Outdated
| stdenv.mkDerivation { | ||
| name = "codex32${shortId}"; | ||
|
|
||
| buildInputs = if doPdfGeneration then [ ghostscript ] else [ ]; |
There was a problem hiding this comment.
I think this ought to be nativeBuildInputs.
There was a problem hiding this comment.
Ok, will change. I really don't understand the distinction.
There was a problem hiding this comment.
My vague understanding is that buildInputs is historic, and means build and runtime dependencies. While nativeBuildInputs is just build time dependencies. I'm not sure what the actual effect of this difference is.
default.nix
Outdated
| buildPhase = '' | ||
| set -e | ||
| ghostscriptTest() { |
There was a problem hiding this comment.
This would make more sense inside checkPhase.
apoelstra
force-pushed
the
2023-03--nixify
branch
from
f3d8d3b to
d4bebf3
Compare
|
Rebased to address commetns. |
default.nix
Outdated
| # Copy output Postscript into place | ||
| mkdir "$out" | ||
| cd "$out" | ||
| ln -s "$FULL_BW" SSS32.ps |
There was a problem hiding this comment.
You changed this from cp to ln -s, but below you patch the file with sed. Does this even work? the content of FULL_BW should be an immutable nix-store file, so I don't think you can patch it.
There was a problem hiding this comment.
Should renderBooklet take a revision argument?
There was a problem hiding this comment.
No, it does not work. The sed line appears to be doing nothing, and since it does nothing, does not even try to write to the file, and returns success.
As for "should renderBooklet just take a revision argument?" let me investigate that. Would be more elegant than rendering the book and then patching it after-the-fact, but might make templating a bit weird.
There was a problem hiding this comment.
After testing this by setting the ref parameter, I believe the existing code does work. I am investigating how. It's as though the ln -s is doing a copy-on-write copy rather than a softlink. (I can confirm that the target file is modified by sed but the original file is not.)
There was a problem hiding this comment.
Oh, sed -i will replace linked files unless you pass it --follow-symlinks.
Linux.
|
Will undraft when this is ready for review again. Hopefully today or tomorrow. |
Pretty-much just adds and removes some newlines in preparation for nix-generating the main PostScript file's page numbering.
apoelstra
force-pushed
the
2023-03--nixify
branch
from
d4bebf3 to
1aebbd5
Compare
|
Restored the old Added Makefile which figures out the git commit ID and passes it to nix. |
|
@roconnor-blockstream @apoelstra I have a (currently failed) attempt to add apoelstra/volvelle-website to nixos-airgapped. My attempt is failed currently because there is some sort of permission error when trying to run Also, is that volvelle-website repo even the correct one to be doing this with? I noticed that this repo seems to have more activity. Do you intend to add some sort of |
Afraid not. I just checked my local nix CI scripts to see if I got wasm-pack working, and the answer is no, I only have wasm-pack code in one place, and it's all commented out with a confusing explanation. Unfortunately wasm-pack is a nightmare to use in an airgapped context, because it downloads random crapand does not respect lockfiles.
Yeah, this repo is where the "real" work is happening. The website is more-or-less a demo and an aid for doing workshops. (I think I will do one in October at TABconf so probably you'll see a bunch of activity there in the week(s) leading up to that, and nothing until then.) But you raise an interesting point that we should have a self-contained "codex32 tool" that people can build and use locally. WASM is nice because you can run it in the browser and easily have nice interactive interfaces, but it's pretty awful in every other respect. One soluion for you would be to just build the site and nix-package the binary output....that'd fly in Nix land even though it wouldn't fly in Guix. But I definitely understand if you refuse on principle to do that!
Probably not, unless we can get away from wasm-pack. (Which honestly shouldn't be too hard ... in theory I just need to run cargo with a wasm32-unknown-unknown target and then write a tiny bit of glue code ... but I don't know how.) |
Thanks for your reply and suggestions. It does give me some comfort that it was not just me who was unable to figure out the wasm-pack stuff with nix! Regarding a self-contained "codex32 tool," agree that would be helpful. |
|
I opened a PR to the website repo with a flake such that |
@VzxPLnHqr: What features are you expecting a self-contained "codex32 tool" to have? I may have already made one. |
@BenWestgate I have not played with it too much yet, so maybe a self-hosted / static version of the website will suffice? (such as as what I added to nixos-airgapped ). But a simple command line interface could be nice too. Is that what you have made already? |
Yes, https://github.com/BenWestgate/codex32/blob/master/reference/python-codex32/src/codex32.py has functions for all sorts of codex32 needs. I started working on a Gtk GUI for importing with error correction: https://github.com/BenWestgate/bails-wallet/blob/master/bails-wallet/string_entry.py I think it was meeting most or all of the criteria of https://github.com/BlockstreamResearch/codex32/blob/master/docs/wallets.md when I last tested it. These were originally for my project https://github.com/BenWestgate/Bails which uses the python library above to support creating codex32 backups and importing them to Bitcoin core. If andrew's bitcoin/bitcoin future PR bitcoin/bitcoin#27351 (comment) gets merged most of that code except my GUI and error correction can be deleted. |
|
|
|
Oh, good catch. This appears in |
apoelstra
force-pushed
the
2023-03--nixify
branch
from
1aebbd5 to
ada141d
Compare
|
Fixed. |
| else | ||
| fetchGit { | ||
| url = ./.; | ||
| inherit ref; |
There was a problem hiding this comment.
After reviewing https://nix.dev/manual/nix/2.22/language/builtins#builtins-fetchGit I'm pretty sure you want rev instead of ref.
There was a problem hiding this comment.
Empirically rev seems to want a 40-character hash, while ref will accept short hashes, branch names, etc.
Maybe I'm being too picky, but you patched the wrong commit. You should have put the
|
|
I don't think you're being too picky. Though I'm surprised I made a mistake like that, given that I used I'll fix the history. Sorry about that. |
Generates the committed postscript and runs diff to make sure that the reconstruction was faithful. Currently this simply reproduces the existing code, though it autogenerates the numbers to make it easier to rearrange things and/or add blank pages. Later PRs will abstract out the setup code, improve best practices (e.g. wrapping every page in "10 dict ... end") and add additional targets which produce e.g. individual worksheets in their own postscript. We will also use the Nix mechanism to add illustrations, rather than using PHP.
apoelstra
force-pushed
the
2023-03--nixify
branch
from
ada141d to
441f7bf
Compare
|
Ah, I see the issue. I was misled by Fixed. |
|
Are you familiar with Bitcoin uses this to insert a define |
|
No, I am not familiar -- though it looks like it needs I can go learn all these things if you want but I feel that my existing way, which behaves predictably and uses explicit code, will be easier to maintain. |
|
|
||
| nativeBuildInputs = if doPdfGeneration then [ ghostscript ] else [ ]; | ||
|
|
||
| phases = [ "buildPhase" ] ++ lib.optionals doCheck [ "checkPhase" ]; |
There was a problem hiding this comment.
Unfortunately the checkPhase is never run because doCheck needs to be set to true in the stdenv.mkDerivation in order to run the checkPhase
You should change the phases line to:
phases = [ "buildPhase" "checkPhase" ];
inherit doCheck;
and let doCheck control whether or not the check phase is executed or not.
| ${lib.optionalString doPdfGeneration "ps2pdf -dPDFSETTINGS=/prepress SSS32.ps"} | ||
| ''; | ||
|
|
||
| checkPhase = toString |
There was a problem hiding this comment.
Unfortunately the check fails when it is actually enabled, at least in part because you try to run ghostscriptTest before it is defined.
|
|
||
| checkPhase = toString | ||
| (map | ||
| (page: "ghostscriptTest ${checkSinglePage page}") |
There was a problem hiding this comment.
There is another issue with this design. Because checkPhase contains references to the single pages, those single pages must be built, whether or not they will be checked. In this case I suggest taking the unusual step of conditionally excluding the entire contents of the checkPhase value if doCheck is false.
Alternatively you could replace fullBooklet.pages with [] when doCheck is false.
| buildPhase = '' | ||
| set -e | ||
| FULL_BW="${renderBooklet fullBooklet}" |
There was a problem hiding this comment.
The way nix derivations work is that, more or less, every value in the derivation becomes an environment variable.
Therefore, it would make more sense for you to define FULL_BW as an entry in the derivation rather that setting it in the build phase.
4 participants
Will also patch the git commit into the "revision" variable which appears in the ps output, if told to build a git repo rather than the current source tree.